Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Various CoDICE L1 Updates #711

Merged
merged 25 commits into from
Aug 15, 2024

Conversation

bourque
Copy link
Collaborator

@bourque bourque commented Jul 24, 2024

Change Summary

Overview

This PR contains various updates to the CoDICE L1 algorithm, mainly to clean some things up, add support for lo-pha and hi-pha data products (event data), and changes required to get recently-acquired simulated data working in the pipeline.

Updated Files

  • imap_processing/cdf/config/imap_codice_global_cdf_attrs.yaml
    • Added CDF attrs for event data products
  • imap_processing/cdf/config/imap_codice_l1a_variable_attrs.yaml
    • Formatting improvements, added missing attributes that are now required
  • imap_processing/cdf/config/imap_codice_l1b_variable_attrs.yaml
    • Formatting improvements, added missing attributes that are now required
  • imap_processing/codice/codice_l0.py
    • Added XTCE mapping for event data
  • imap_processing/codice/codice_l1a.py
    • Added support for event data; moved housekeeping dataset creation into module to be consistent with where similar code is being defined; other various cleanup/organizing
  • imap_processing/codice/constants.py
    • Moved hard-coded APID list from codice_l1a to constants file
  • imap_processing/codice/packet_definitions/*.xml
    • Updates from newer version of space_packet_parser and updates packet definition spreadsheet
  • imap_processing/codice/utils.py
    • Moved create_hskp_dataset to codice_l1a.py to be consistent with where the code to create other datasets resides
  • imap_processing/tests/codice/data/imap_codice_l0_*.pkts
    • Updated/new simulated data
  • imap_processing/tests/codice/test_codice_l0.py
    • Renamed functions with hskp to be consistent with naming convention
  • imap_processing/tests/codice/test_codice_*.py
    • Improved formatting and updated tests

@bourque bourque added Ins: CoDICE Related to the CoDICE instrument Level: L1 Level 1 processing labels Jul 24, 2024
@bourque bourque self-assigned this Jul 24, 2024
Comment on lines +3 to +17
CATDESC: ""
DISPLAY_TYPE: no_plot
FIELDNAM: ""
FILLVAL: -9223372036854775808
FORMAT: I12
LABLAXIS: ""
REFERENCE_POSITION: ""
RESOLUTION: ""
SCALETYP: linear
TIME_BASE: ""
TIME_SCALE: ""
UNITS: dN
VALIDMIN: -9223372036854775808
VALIDMAX: 9223372036854775807
VAR_TYPE: data
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was just to have consistent indenting across the file.

Comment on lines +55 to +66
DISPLAY_TYPE: ""
FIELDNAM: Energy step
FILLVAL: ""
FORMAT: A3
LABLAXIS: ""
REFERENCE_POSITION: ""
RESOLUTION: ""
TIME_BASE: ""
TIME_SCALE: ""
UNITS: ""
VALIDMAX: ""
VALIDMIN: ""
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found that I needed to add these in order to pass the new check here:

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, is that a bug in the validation code? Based on the schema YAML here, I wouldn't expect all of these additions to be necessary.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I am understanding correctly, this is checking against whatever is defined as "required" in the default_variable_cdf_attrs_schema.yaml file. It loos like all of these are indeed required according to this file, for example TIME_BASE:

attribute_key:
  # ===== EPOCH-ONLY VARIABLE ATTRIBUTES =====
  TIME_BASE:
    description: >
      fixed (0AD, 1900, 1970 (POSIX), J2000 (used by CDF_TIME_TT2000),
      4714 BC (Julian)) or flexible (provider-defined)
    required: true    # NOT Required in ISTP Guide

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. I am wondering if it should only be checking what is required and is present in the list of attributes defined for the datatype. Not important to this PR though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on slack convos, sounds like Maxine will fix this eventually. I will hang tight on merging this PR and update accordingly when the fix is in.

@bourque bourque requested review from a team, greglucas, subagonsouth and tech3371 and removed request for a team July 24, 2024 17:55
Copy link
Contributor

@subagonsouth subagonsouth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This mostly looks good to me. I didn't review the XTCE changes.

Comment on lines +55 to +66
DISPLAY_TYPE: ""
FIELDNAM: Energy step
FILLVAL: ""
FORMAT: A3
LABLAXIS: ""
REFERENCE_POSITION: ""
RESOLUTION: ""
TIME_BASE: ""
TIME_SCALE: ""
UNITS: ""
VALIDMAX: ""
VALIDMIN: ""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, is that a bug in the validation code? Based on the schema YAML here, I wouldn't expect all of these additions to be necessary.

imap_processing/codice/codice_l1a.py Show resolved Hide resolved
imap_processing/tests/codice/test_codice_l1a.py Outdated Show resolved Hide resolved
imap_processing/codice/codice_l1a.py Show resolved Hide resolved
Copy link
Collaborator

@greglucas greglucas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comments is all. There are a lot of refactor things going on, but no new processing from what I could tell. If you could break up the yaml/metadata additions and code refactor in future PRs I think that would be helpful too.

I am hopeful that going to a combined XTCE with dynamic variable length and starting from the xarray datasets will be helpful for you here.

Note that we aren't including the short/long description in the housekeeping datasets automatically and planning on using yaml configuration files for all metadata.
See this thread for more discussion: #687 (comment)


# Write the dataset to a CDF so it can be manually inspected as well
file_path = write_cdf(dataset)
print(f"CDF file written to {file_path}")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
print(f"CDF file written to {file_path}")

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose I should change this to a logging statement. I like to have this path printed so that I can copy over the resulting CDF in case I want to manually inspect it.

@bourque
Copy link
Collaborator Author

bourque commented Jul 26, 2024

pre-commit.ci autofix

@bourque
Copy link
Collaborator Author

bourque commented Aug 14, 2024

I added a TODO in the code to remind myself that the CDF variable attributes config needs to be updated once the problem with the cdf_attribute_manager checks is resolved. I am going to merge this now so that I can start working off of this code to make more updates.

@bourque
Copy link
Collaborator Author

bourque commented Aug 14, 2024

pre-commit.ci autofix

@bourque bourque merged commit b3d0377 into IMAP-Science-Operations-Center:dev Aug 15, 2024
17 checks passed
@bourque bourque deleted the codice-l1-updates branch August 15, 2024 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ins: CoDICE Related to the CoDICE instrument Level: L1 Level 1 processing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants